Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nixos/yazi: support plugins and flavors #295846

Merged
merged 2 commits into from
May 18, 2024
Merged

nixos/yazi: support plugins and flavors #295846

merged 2 commits into from
May 18, 2024

Conversation

linsui
Copy link
Contributor

@linsui linsui commented Mar 14, 2024

Description of changes

And fix the crash.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@sxyazi

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Mar 14, 2024
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Mar 14, 2024
@sxyazi
Copy link

sxyazi commented Mar 14, 2024

Thanks for the fix!

@XYenon can you take a look at this PR?

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/3663

This was referenced Mar 31, 2024
@teto
Copy link
Member

teto commented Apr 3, 2024

I would prefer to see plugin support in pkgs/by-name/ya/yazi/package.nix, modules should be kind of last resort IMO. The module could keep the extensions but the implementation should live in the wrapper.
It would makes the features accessible outside of nixos (includign home-manager)

@linsui
Copy link
Contributor Author

linsui commented Apr 4, 2024

Do you want to replace the module with a wrapper? Why is a wrapper preferred? I thought the wrapper will just become a mess. Those args in the wrapper can't be documented and discovered and no one knows how to use them without reading the code (I have seen enough examples). HM should implement their own module with plugins from nixpkgs.

@Aleksanaa Aleksanaa requested a review from XYenon April 4, 2024 10:12
@teto
Copy link
Member

teto commented Apr 4, 2024

Do you want to replace the module with a wrapper?

Not necessarily, we can have both if you maintain the module.

Why is a wrapper preferred?

Because it can be used outside nixos.

Those args in the wrapper can't be documented and discovered and no one knows how to use them without reading the code

says who ? of course they can be documented, just look at the nixpkgs documentation. Not only that but it's a lot easier to test a wrapper than a module.

@linsui
Copy link
Contributor Author

linsui commented Apr 5, 2024

says who ? of course they can be documented, just look at the nixpkgs documentation. Not only that but it's a lot easier to test a wrapper than a module.

Right, the only problem is that only few of them are documented because it's not generated automatically.

Because it can be used outside nixos.

Do you have any idea how to implement it? I thought we have to set YAZI_CONFIG_HOME in the wrapper and this overrides the default config files. So if the plugins are installed with the wrapper then the config also needs to be set with the wrapper. And this module will become a wrapper of the wrapper? Or a generator of the config home directory?

@teto
Copy link
Member

teto commented Apr 6, 2024

I thought we have to set YAZI_CONFIG_HOME in the wrapper and this overrides the default config files. So if the plugins are installed with the wrapper then the config also needs to be set with the wrapper.

This could be seen as an advantage, i.e, you get a pure config. but I recognize this can be a bother. Isn't it the same with this PR ? aka if you set files in YAZI_CONFIG_HOME = /etc/yazi then yazi will ignore the files in $XDG_CONFIG_HOME/yazi ?

There is a similar issue with the neovim wrapper.
The wrapper could (in impure mode, if we ever want that) generate an init.lua file that chain loads the one in $XDG_CONFIG_HOME ?

@linsui
Copy link
Contributor Author

linsui commented Apr 7, 2024

aka if you set files in YAZI_CONFIG_HOME = /etc/yazi then yazi will ignore the files in $XDG_CONFIG_HOME/yazi ?

Yazi by default only loads the config files in home.

The wrapper could (in impure mode, if we ever want that) generate an init.lua file that chain loads the one in $XDG_CONFIG_HOME ?

I don't think that yazi can load a config file in a config file...

OK, I know how to do that now.

@linsui
Copy link
Contributor Author

linsui commented Apr 7, 2024

I find a stupid problem. In fact I don't use the wrapper at all because I don't use those tools in the annoying wrapper. If I use the wrapper I have to set all of them to false. If the config is added to the wrapper I have to use the wrapper. This provide no benefits to me. So I add another option to disable all of them.

@ofborg ofborg bot requested a review from matthiasbeyer April 7, 2024 16:02
@ofborg ofborg bot added 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin labels Apr 7, 2024
@linsui linsui requested a review from teto April 11, 2024 05:26
@linsui
Copy link
Contributor Author

linsui commented Apr 27, 2024

@teto Is there anything I can do? :)

@ofborg ofborg bot requested a review from XYenon May 17, 2024 18:52
@Aleksanaa Aleksanaa merged commit 419fffe into NixOS:master May 18, 2024
24 of 25 checks passed
@linsui linsui deleted the yazi branch May 18, 2024 05:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants